typescript

Why append to the id, you could just use SingletonLambda as the id.

I found it easier to track the deployed stacks in CFN by having a common prefix in the stack ID. Otherwise, its difficult to keep track

Use lambda.Code.asset(codeLocation) instead of reading yourself.

The dynamodb:* permissions should be offered as static methods, either in this package or @aws-cdk/aws-dynamodb.

Again, why append to the id?

See above

Why do we need a new package as opposed to @aws-cdk/aws-dynamodb?

Made more sense to me to have a new package since it has different requirements than the regular aws-dynamodb table - i.e. required tableName and streamSpecification. As well as its a higher level object than the dynamodb package.

Once CFN officially supports global-dynamodb tables, it would probably be easier to deprecate this package than remove it from an integrated portion of the aws-dynamodb package

I think it's fine to have this as a separate module. Maybe we can add a comment in the dynamodb README that mentions it

This is not idiomatic, pass the properties directly when creating the construct:

new GlobalTable(stack, 'aws-cdk-dynamodb-global', {
    dynamoProps: {
        partitionKey: TABLE_PARTITION_KEY,
        tableName: TABLE_NAME
    },
    regions: [ 'us-east-1', 'us-east-2', 'us-west-2' ],
    // etc.
});

Is there documentation on the recommended practice from AWS?

AWS has no recommended practice for this since its not supported in CFN currently. This is the solution determined by myself and by chatting with CDK contributors

These consts are not really adding much in terms of readability and they are only used once, just inline them

Make sure you capitalize service names properly and use the prefix "Amazon" or "AWS" (in this case it's Amazon DynamoDB).

You can extract all the props besides tableName from TableProps into TableOptions interface and then have GlobaProps and TableProps inherit from Options (that's kind of how we do this)

Probably should error if users specified something else

Rename to GlobalTableProps

Sorry, I probably didn't explain myself too well (interfaces can extend multiple bases):

export interface GlobalTableProps extends cdk.StackProps, dynamodb.TableOptions {
  tableName: string;
  regions: string[];
}

Then, you don't need GlobalDynamoDBProps at all.

We like flat ;-)

Short description of this type (the README intro is great)

Do we need to export this module? Seems like an internal implementation

I think it's fine to have this as a separate module. Maybe we can add a comment in the dynamodb README that mentions it

Why are these consts needed?

I think the API should only expose the Table objects and not the stack that wraps them. Furthermore, I don't even thing you need this MultiDynamoDBStack class (and of course you don't need it in the public API). You can just create those stacks in-place:

for (const reg of props.regions) {
  const regionalStack = new cdk.Stack(this, '...', { env: { region: reg } });
  const regionalTable = new dynamodb.Table(regionalStack, 'Table', props);
  this.tables.push(regionalTable);
}

This is how you should define the property:

public readonly tables = new Array<dynamodb.Table>();

P.S. this is not ideal because the array is mutable. The "right" thing to do would be to return a clone:

private readonly _regionalTables = new Array<dynamodb.Table>();

public get regionalTables() {
  return this._regionalTables.map(x => x);
}

Chiming in—if mutability is a concern, would it make sense to use a library like https://github.com/immutable-js/immutable-js, or would that be too big of a dependency to expose as a public API in the CDK?

We prefer to avoid taking such dependencies (and frankly JS/TS are sufficient). Soon Typescript will have x: readonly string[] and the world will be whole.

call this regionalTables

Please don't check in the handler.zip file! Where is the code and how is it bundled into a zip file?

That's been addressed now

2 spaces please

Wording here feels off. "Enforces?" Ideally if we can copy & paste from official AWS docs that would be better. I would write something like "Name of the DynamoDB table to use across all regional tables. This is required for global tables".

This describes Global Tables but the class is RegionalTables?

Why is this called "RegionalTables" instead of "GlobalTable"?

You asked for the class to be renamed?
#2082 (comment)

Not the class, the property name. It's: GlobalTable.regionalTables. Otherwise it doesn't make sense...

nit: Remove space after "("

Does it makes sense that update is a no-op? What if I add a region for example?

You must catch all exceptions and send a FAILED response. Otherwise, any exception will cause the CFN deployment to timeout after 4 hours.

Then, all failure cases should just "throw".

Ideally, if you can also have only a single point in your code that sends a SUCCESS response, it would be safer, especially if someone updates this code in the future and forgets to handle some case (alternatively, "throw" when you fall through).

Why is this needed?

Please add a unit test for this handler

Not enough.. You'll have to mock stuff..

lol I know, just checked in what I had gotten done so far this morning :)